Skip to content

PYTHON-4780 Implement fast path for server selection with Primary #2416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 3, 2025

Tested with the following script:

from pymongo import MongoClient
from pymongo.read_preferences import ReadPreference
from concurrent.futures import ThreadPoolExecutor, wait

client = MongoClient()
session = client.start_session()
read_preference = ReadPreference.PRIMARY


def test():
    for i in range(100):
        client._select_server(read_preference, session, "testOperation")



def test2_target():
    client._select_server(read_preference, session, "testOperation")


def test2():
    futures = []
    with ThreadPoolExecutor(max_workers=100) as pool:
        for i in range(100):
            futures.append(pool.submit(test2_target))
        wait(futures)
    

if __name__ == '__main__':
    import timeit
    print(timeit.timeit("test()", setup="from __main__ import test, client, read_preference, session", number=1000))
    print(timeit.timeit("test2()", setup="from __main__ import test2, client, read_preference, session", number=1000))

On master:
0.4806627919897437
1.5110677920019953

With this change:
0.17988054199668113
1.233099208009662

@blink1073
Copy link
Member Author

I didn't schedule a perf build because our perf tests only run on standalone.

@blink1073 blink1073 requested a review from ShaneHarvey July 3, 2025 15:34
@blink1073 blink1073 marked this pull request as ready for review July 3, 2025 15:34
@blink1073 blink1073 requested a review from a team as a code owner July 3, 2025 15:34
@ShaneHarvey
Copy link
Member

ShaneHarvey commented Jul 3, 2025

Minor note for the benchmark, thread dispatching could be dominating test2. Could you change it to pool.submit(test) and just remove test2_target? That would probably show a larger perf increase for threaded.

@blink1073
Copy link
Member Author

With your suggested benchmark changes:

On master:
0.4840990829980001
0.5537169170274865

With this PR:
0.1950377920002211
0.260104083979968

@blink1073 blink1073 requested a review from ShaneHarvey July 8, 2025 16:57
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Could you add a changelog saying "Improved performance of selecting a server with the Primary selector."?

@@ -324,6 +324,16 @@ def apply_selector(
description = self.server_descriptions().get(address)
return [description] if description else []

# Primary selection fast path.
if self.topology_type == TOPOLOGY_TYPE.ReplicaSetWithPrimary and isinstance(
selector, Primary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My one concern is that technically this could be a breaking change if a user is subclassing Primary and overriding __call__ to do something special during server selection. One simple way to avoid that risk is doing type(selector) is Primary rather than isinstance. Or we could document the potential breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the code modification option

@blink1073 blink1073 requested a review from ShaneHarvey July 8, 2025 18:11
if self.topology_type == TOPOLOGY_TYPE.ReplicaSetWithPrimary and type(selector) is Primary:
for sd in self._server_descriptions.values():
if sd.server_type == SERVER_TYPE.RSPrimary:
return [sd]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh one more thing, don't we have to call the custom_selector here if one was provided?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And add a test for it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to defer for now, working on a HELP ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants